Conversation
|
@Niek Now that the refactor has been merged, can you review this pull request? |
There was a problem hiding this comment.
Pull request overview
This PR introduces a Puppeteer-independent “core” entry point so consumers can use path generation without pulling in puppeteer types, while keeping the existing Puppeteer-focused API as the default export.
Changes:
- Added
src/core.tscontainingpath()andPathOptions(no Puppeteer imports). - Refactored
src/spoof.tsto import/re-exportpath()andPathOptionsfromcore. - Added a package subpath export (
ghost-cursor/core) viapackage.json#exports.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/spoof.ts | Removes in-file path() implementation and re-exports it from the new core module. |
| src/math.ts | Introduces a Rectangle type used by core to avoid Puppeteer BoundingBox typing. |
| src/core.ts | New Puppeteer-free module containing path generation logic and related types. |
| package.json | Adds exports map for . (spoof) and ./core entry points. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| "exports": { | ||
| ".": { | ||
| "types": "./lib/spoof.d.ts", | ||
| "default": "./lib/spoof.js" | ||
| }, | ||
| "./core": { | ||
| "types": "./lib/core.d.ts", | ||
| "default": "./lib/core.js" | ||
| }, |
There was a problem hiding this comment.
PR metadata/issue link indicates this should fix TS users who only import path() without having puppeteer installed, but the root export (".") still points to ./lib/spoof.d.ts, which imports puppeteer types. That means import { path } from 'ghost-cursor' will still fail unless consumers switch to ghost-cursor/core. Consider either (a) making the root export point to core and adding an explicit ./spoof subpath for Puppeteer-specific APIs, or (b) updating public docs (README/changelog) to direct non-Puppeteer users to ghost-cursor/core so the linked issue is actually resolved for readers.
There was a problem hiding this comment.
It depends on the package's main purpose.
-
Humanize mouse movements in Puppeteer (and secondary generate human-like movement).
{ ".": "./lib/spoof.js", "./core": "./lib/core.js" } -
Generate human-like movement (and secondary provide integration for Puppeteer).
{ ".": "./lib/core.js", "./puppeteer": "./lib/spoof.js" }
With the second solution: there is a breaking change, but it’s easier to add a new integration.
| }, | ||
| "./core": { | ||
| "types": "./lib/core.d.ts", | ||
| "default": "./lib/core.js" | ||
| }, |
There was a problem hiding this comment.
Adding a package exports map restricts consumers from importing any previously-available deep paths (e.g. ghost-cursor/lib/... or ghost-cursor/mouse-helper) unless they are explicitly listed. If this package has downstream users relying on those deep imports, this becomes a breaking change. Consider explicitly exporting any intended public subpaths (at least ./spoof, ./mouse-helper, etc.) or doing this behind a major version bump.
| }, | |
| "./core": { | |
| "types": "./lib/core.d.ts", | |
| "default": "./lib/core.js" | |
| }, | |
| }, | |
| "./spoof": { | |
| "types": "./lib/spoof.d.ts", | |
| "default": "./lib/spoof.js" | |
| }, | |
| "./core": { | |
| "types": "./lib/core.d.ts", | |
| "default": "./lib/core.js" | |
| }, | |
| "./lib/*": "./lib/*", |
There was a problem hiding this comment.
I don't think people imported these directly, since they are not documented. Right, @regseb?
There was a problem hiding this comment.
Yes, it's unlikely that anyone would import a deeply nested file.
- It's not common in npm to import deeply nested files.
- The existence of these files isn't documented.
- I don't see what functionality a user would want to access in these files.
| /** Generates a set of points for mouse movement between two coordinates. */ | ||
| export function path ( | ||
| start: Vector, | ||
| end: Vector | Rectangle, | ||
| /** | ||
| * Additional options for generating the path. | ||
| * Can also be a number which will set `spreadOverride`. | ||
| */ | ||
| // TODO: remove number arg in next major version change, fine to just allow `spreadOverride` in object. | ||
| options?: number | PathOptions): Vector[] | TimedVector[] { | ||
| const optionsResolved: PathOptions = typeof options === 'number' | ||
| ? { spreadOverride: options } | ||
| : { ...options } | ||
|
|
||
| const DEFAULT_WIDTH = 100 | ||
| const MIN_STEPS = 25 | ||
| const width = 'width' in end && end.width !== 0 ? end.width : DEFAULT_WIDTH | ||
| const curve = bezierCurve(start, end, optionsResolved.spreadOverride) | ||
| const length = curve.length() * 0.8 | ||
|
|
||
| const speed = optionsResolved.moveSpeed !== undefined && optionsResolved.moveSpeed > 0 | ||
| ? (25 / optionsResolved.moveSpeed) | ||
| : Math.random() | ||
| const baseTime = speed * MIN_STEPS | ||
| const steps = Math.ceil((Math.log2(fitts(length, width) + 1) + baseTime) * 3) | ||
| const re = curve.getLUT(steps) | ||
| return clampPositive(re, optionsResolved) | ||
| } |
There was a problem hiding this comment.
src/core.ts introduces a new public entry point (ghost-cursor/core) and moves core path-generation logic there, but the test suite currently only exercises spoof behavior. Add a small unit test that imports from ../core and asserts basic invariants of path() output (e.g., starts/ends near the requested points, returns non-negative coordinates, timestamps strictly increase when useTimestamps is enabled) to prevent regressions and ensure this entry point stays Puppeteer-free.
There was a problem hiding this comment.
@copilot create a unit test based on this feedback. make sure to test the most common edge-cases.
Fix #152
Split features into two files:
corefor puppeteer independent features :path()spoof(default) for puppeteer related features and alsocore:createCursor(),path()